Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[♻] {PROD4POD-1931} Move zipfile mock-up and improve coverage #1205

Merged
merged 15 commits into from
Oct 14, 2022

Conversation

JJ
Copy link
Contributor

@JJ JJ commented Oct 4, 2022

✍️ Description

The mock for zip-files is more useful in the common poly-import module. That way it can also be used in the module itself to mock zipfiles, and imported from any module that needs to mock zips.

While we were at it, boost a bit coverage of said test.

🏗️ Fixes PROD4POD-1931

It is a precondition for #1204, since Storage needs zip files to be tested.

ℹ️ Other information

Some minor refactoring also made.

Might include some refactoring of tests, mainly to reduce boilerplate. Not for the time being.

Additional functionality

  • ✅ adds tests in parts that were not covered, for instance, file name analysis in Facebook

♥️ Thank you!

@JJ JJ changed the title [♻] {PROD4POD-1930} Move zipfile mock-up [♻] {PROD4POD-1930} Move zipfile mock-up and improve coverage Oct 4, 2022
@JJ JJ changed the title [♻] {PROD4POD-1930} Move zipfile mock-up and improve coverage [♻] {PROD4POD-1931} Move zipfile mock-up and improve coverage Oct 4, 2022
Copy link
Contributor

@fhd fhd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM - but it probably makes sense for Richard to take a look since he wrote at least some of it.


it("passes through non-string data", () => {
const foo42 = { foo: 42 };
expect(jsonStringifyWithUtfEscape(foo42)).toBe('{"foo":42}');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd do: expect(jsonStringifyWithoutUtfEscape(foo42)).toBe(JSON.stringify(foo42)) - to be more clear that it's just a normal stringify without UTF-8 data. But no strong opinion, being explicit with the values is also good.

let status = null;

beforeAll(async () => {
let zipFile = new ZipFileMock();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: For variables that don't get reassigned, we generally used const (i.e. wherever possible).

@JJ JJ merged commit a580e09 into main Oct 14, 2022
@JJ JJ deleted the ♻-move-zipfile-mock-up branch October 14, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants